Paginate by creation time instead of key order by benthecarman · Pull Request #96 · lightningdevkit/vss-server

benthecarman · 2026-04-02T01:55:50Z

Clients often need to fetch recent payments or entries without iterating over the entire keyspace. Ordering by created_at lets callers retrieve the newest records first and stop early, which is not possible with lexicographic key ordering.

The page token now encodes (created_at, key) so the cursor remains unique even when multiple rows share the same timestamp. A composite index on (user_token, store_id, created_at, key) keeps the query efficient, and a migration back-fills any NULL created_at values and adds the NOT NULL constraint.

ldk-reviews-bot · 2026-04-02T01:55:53Z

👋 Thanks for assigning @tankyleo as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

ldk-reviews-bot · 2026-04-04T01:56:25Z

🔔 1st Reminder

Hey @tankyleo! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

tankyleo

thanks !

tankyleo

We'll want to update types.rs to require this ordering too

benthecarman · 2026-04-04T20:12:06Z

responded to review

tankyleo · 2026-04-06T18:29:42Z

 message ListKeyVersionsResponse {

-  // Fetched keys and versions.
+  // Fetched keys and versions, ordered by creation time (oldest first).


To confirm here, the server is free to choose any ordering they would like in case two keys have the same timestamp.

We have the server sort by key after timestamp. I'll add that here

Yes, but I don't this is a requirement on the VSS API right ? Ie clients don't care what ordering is picked beyond newest first.

I think it's better to me more explicit than less. We have to define a tiebreaker anyways so may as well put it in the docs

Let me see this defines an API constraint right, in addition to just documentation ? For sure I see the use for documentation here, but I don't want clients to start relying on this behavior.

tankyleo · 2026-04-06T18:44:12Z

please request review when you are ready so i don't miss this PR

tnull · 2026-04-08T07:36:48Z

  // Use this value to query for next-page of paginated `ListKeyVersions` operation, by specifying
  // this value as the `page_token` in the next request.
  //
-  // If `next_page_token` is empty (""), then the "last page" of results has been processed and


No, I think we were intentionally following protobuf's/Google's best practices here. https://google.aip.dev/158 states:

Response messages for collections should define a string next_page_token field, providing the user with a page token that may be used to retrieve the next page.

The field containing pagination results should be the first field in the message and have a field number of 1. It should be a repeated field containing a list of resources constituting a single page of results.

If the end of the collection has been reached, the next_page_token field must be empty. This is the only way to communicate "end-of-collection" to users.

If the end of the collection has not been reached (or if the API can not determine in time), the API must provide a next_page_token.

IMO would be good to revert and include this context in the docs.

Okay updated

ldk-reviews-bot · 2026-04-09T03:36:27Z

🔔 1st Reminder

Hey @tankyleo! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

tankyleo

Rushed some comments out, will be back after lunch.

tankyleo · 2026-04-09T19:42:38Z

+		}
+
+		let first_page = ctx.list(None, Some(page_size), None).await?;
+		assert_eq!(first_page.key_versions.len(), page_size as usize);


Given the VSS API, the page could have length 0 here. As long as the page token is not empty, the client would be expected to make another request with the new page token.

I'm confused what you're saying here. This is the first page we are requesting, the only way we'd a length of 0 would be if we had no items.

Just that I think a VSS-server is within bounds if it returns a response with an empty list, but a non-empty token. The client would be expected to continue asking for pages.

I'm mostly going from this line in the docs of ListKeyVersionRequest:

/// `page_size` is used by clients to specify the maximum number of results that can be returned by /// the server. /// The server may further constrain the maximum number of results returned in a single page. /// If the `page_size` is 0 or not set, the server will decide the number of results to be returned. #[prost(int32, optional, tag = "3")] pub page_size: ::core::option::Option<i32>,

TLDR can't assume the page you get back is the same length as the page size in your ListKeyVersionsRequest

TLDR can't assume the page you get back is the same length as the page size in your ListKeyVersionsRequest

This is true.

if it returns a response with an empty list, but a non-empty token.

But there is no reason to respond with empty list and non-empty pagination-token. (I don't think this should happen, can add a kvstore test/assert for it if doesn't exist already.)

tankyleo · 2026-04-09T20:52:19Z

Done with this pass here, just needed to add the comment about the VSS server API constraint

G8XSU · 2026-04-09T21:06:07Z

Adding some historical context, VSS was purposefully built to be storage engine agnostic. The VSS protocol/API/data-model doesn't enforce a specific underlying storage database.

We can totally revisit this requirement and say VSS only works with postgres-like relational databases. But extending pagination to order by creation_time limits the capability to use any modern pure KV store as an underlying database engine like DynamoDB or CosmosDB.

I understand the need to efficiently paginate through large result sets. A couple of alternatives that preserve storage engine agnosticism:

Clients could use ordered key IDs like UUIDv7, which gives you time-based ordering for free
via lexicographic sort on the key itself.
Clients could scope their list_key_versions calls to known key prefixes to keep result sets
manageable.
Cursor-based pagination on the key (lexicographic ordering) works on every KV store without
requiring timestamps.

benthecarman · 2026-04-09T22:05:05Z

Fixed @tankyleo comments about tests

tankyleo · 2026-04-09T22:13:53Z

@G8XSU Thank you for the feedback, I'll be considering the tradeoffs over the next few days.

Was wondering do you have an email where I can reach you ? Feel free to ping me at "hello at leonash dot net" I was wondering if we could get the vss-client crate at https://crates.io/crates/vss-client from you.

benthecarman · 2026-04-09T22:16:22Z

@G8XSU my perspective

Clients could use ordered key IDs like UUIDv7, which gives you time-based ordering for free
via lexicographic sort on the key itself.

in theory, yes, but ldk-node isn't using this and migrating would be a huge lift

Clients could scope their list_key_versions calls to known key prefixes to keep result sets
manageable.

ldk-node uses txid/payment_hash as key so we can't really use prefixes

Cursor-based pagination on the key (lexicographic ordering) works on every KV store without
requiring timestamps.

Yes, but then the pagination isn't really useful anymore. You just end up getting a random set of payments rather an actual ordered list.

We can totally revisit this requirement and say VSS only works with postgres-like relational databases. But extending pagination to order by creation_time limits the capability to use any modern pure KV store as an underlying database engine like DynamoDB or CosmosDB.

imo postgres scales really well and you're scaling past something like postgres then it's likely you aren't using the off the shelf solution anymore and will have a whole team to manage things like stuff. And being able to sort by something useful rather than just lexicographical is worth it

tankyleo

LGTM

Right, but if it's not part of the API guarantees, isn't it odd to lean on/assume that the client only wants global version. At the very least the comment is misleading, IMO.

Let's just address this comment from tnull thank you, perhaps something like "page 0 means we get to decide"

benthecarman · 2026-04-15T20:53:04Z

Updated the comment about 0 page size

tnull

Generally fine by me I think, one comment.

Aside from that, I do however wonder if we should finally add a protocol version to VSS? While this is not an API breaking change, clients will have no good idea whether they can expect pagination to work when they connect to a particular backend?

@tankyleo Any thoughts?

tnull · 2026-04-16T08:53:11Z

 message ListKeyVersionsResponse {

-  // Fetched keys and versions.
+  // Fetched keys and versions, ordered by creation time (newest first).


Remind me, is there any particular reason we need to use time for this? Wouldn't we get around the need for a tie breaker if we'd simply use a monotonically increasing atomic counter instead (e.g., a Postgres BIGSERIAL column) ? Then we'd be certain than each entry has a unique value?

Claude:

Monotonic counter vs. creation_time Your intuition is right — a monotonically increasing counter (e.g., a Postgres BIGSERIAL column) would be strictly simpler here. The current approach has to deal with: - Tie-breaking: The compound condition (created_at < $3 OR (created_at = $3 AND key > $4)) in the SQL query - A composite page token: Encoding both the timestamp and the key (0:<micros>:<key>) - A composite index: (user_token, store_id, created_at DESC, key ASC) INCLUDE (version) - A dedicated test just for the tie-breaking behavior With an auto-incrementing counter (say row_id BIGSERIAL), all of that collapses to: - WHERE row_id < $cursor ORDER BY row_id DESC LIMIT $N - Page token = just the counter value - Simple single-column addition to the index - No tie-breaking needed since values are unique by definition The only argument for creation_time would be if it carried semantic meaning the client cares about (e.g., "show me keys created after X"). But looking at the ListKeyVersionsResponse, created_at isn't exposed to the client — it's purely an internal ordering mechanism. So you're paying the complexity tax of timestamps without getting the semantic benefit.

Thank you tnull for raising this point. I did some back-and-forth with claude, and yea this is interesting !

My one question at this point is backwards compat. Do you have thoughts on this point ? I'm thinking if keys created before this commit don't have strict creation order, this is OK. Perhaps we can use the VSS version you described above to encourage people to upgrade once we start relying on PaginatedKVStore in LDK Node.

@benthecarman let me know what you think.

@tnull I ping you on the response above in case it helps bubble this up in your inbox :)

Ah, I didn't know we could use BIGSERIAL for non-primary keys (in sqlite you can't).

Yeah this might be better. On backwards compat, we still should be able to backfill the column by sorting by creation time, however, the migration that claude generated for this is pretty big/ugly.

Thank you tnull for raising this point. I did some back-and-forth with claude, and yea this is interesting !

My one question at this point is backwards compat. Do you have thoughts on this point ? I'm thinking if keys created before this commit don't have strict creation order, this is OK. Perhaps we can use the VSS version you described above to encourage people to upgrade once we start relying on PaginatedKVStore in LDK Node.

@benthecarman let me know what you think.

I think the backfill should still handle it for the most part? But yeah, apart from that I think it might be good to lean on the versioning byte for this going forward, and make sure we publish vss-server v0.1 (with protocol versioning support) prior to LDK Node v0.8?

benthecarman · 2026-04-16T20:23:24Z

I do however wonder if we should finally add a protocol version to VSS?

Yeah i think this makes sense probably for a follow up tho

benthecarman · 2026-04-16T20:30:46Z

Pushed to use the monotonic counter. Just did the migration by adding the column and letting postgres backfill it in scan order. Doing it ourselves by sorting by creation time got really ugly and after talking with @tankyleo, didn't seem worth it.

tankyleo

LGTM will take another pass tomorrow

ldk-reviews-bot · 2026-04-20T00:00:51Z

🔔 1st Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

tnull

Generally looks good I think, but there are some follow-up changes to we should do, now that we don't use creation time anymore.

tnull · 2026-04-20T09:03:46Z

 message ListKeyVersionsResponse {

-  // Fetched keys and versions.
+  // Fetched keys and versions, ordered by creation time (newest first).


These docs are now inaccurate, no?

Our implementation of this contract changed, but the contract remains the same, I think this is still accurate. Same for the other comments below.

tnull · 2026-04-20T09:04:24Z

 		Ok(())
 	}

+	async fn list_should_return_results_ordered_by_creation_time() -> Result<(), VssError> {


Same here and below, the test names seems inaccurate now that we don't sort by time?

tnull · 2026-04-20T09:05:21Z

 #[derive(Clone, PartialEq, ::prost::Message)]
 pub struct ListKeyVersionsResponse {
-	/// Fetched keys and versions.
+	/// Fetched keys and versions, ordered by creation time (newest first).


Also inaccurate now.

tnull · 2026-04-20T09:07:40Z

 const VERSION_COLUMN: &str = "version";
+const SORT_ORDER_COLUMN: &str = "sort_order";
+
+const CURRENT_PAGE_TOKEN_VERSION: char = '0';


Hmm, I think now that we don't have extra semantics, we should be good to drop the page token versioning byte and all associated logic again and simply use the sort_order as page token? (Especially given that we're discussing adding a protocol-level version, which we could also use if we'd ever find that we need to switch page token semantics again?)

Makes sense to me yes given the upcoming protocol-level version

tnull · 2026-04-20T09:09:24Z

 VSS ships with a PostgreSQL implementation by default and can be hosted in your favorite infrastructure/cloud provider
-(AWS/GCP) and its backend storage can be switched with some other implementation for KeyValueStore if needed.
+(AWS/GCP). The backend storage can be switched with another implementation, but it must support ordering by creation
+time, a simple key-value store is not sufficient.


No, we don't sort by creation time anymore. Might be good to be more accurate here, too.

benthecarman · 2026-05-04T01:20:51Z

@tnull all your comments here are wrong it seems, we still do sort by creation time, we just use a counter in the db.

tnull · 2026-05-04T07:16:00Z

@tnull all your comments here are wrong it seems, we still do sort by creation time, we just use a counter in the db.

I guess it depends on how literal you want to take the term 'creation time'? Fine to leave the docs if you think the slight semantic difference doesn't matter, but IMO we should still drop the now-unnecessary semantics in the page token itself (#96 (comment))?

Clients often need to fetch recent entries without iterating over the entire keyspace. Ordering by a monotonic insertion counter lets callers retrieve the newest records first and stop early, which is not possible with lexicographic key ordering. A BIGSERIAL sort_order column is added to vss_db. New rows get a monotonically increasing value from its sequence, and list queries order by sort_order DESC. Because sort_order is UNIQUE, the page token collapses to a single integer with no tiebreaker needed. A composite index on (user_token, store_id, sort_order DESC) INCLUDE (key, version) keeps list queries as index-only scans. Pre-existing rows receive sequence values in heap-scan order during the column rewrite, so their list ordering will not reflect creation time; new rows onward do. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

benthecarman · 2026-05-04T14:29:29Z

Okay removed version number from page token

tankyleo

I ran this through gpt-5.5 xhigh, and it found these two things, sorry I didn't do this earlier.

Medium: next_page_token now exposes the raw global sort_order. Since sort_order is a global BIGSERIAL UNIQUE (rust/impls/src/migrations.rs:38) and the token is just sort_order.to_string() (rust/impls/src/postgres_store.rs:40), clients can infer service-wide write volume and gaps caused by other
tenants. For a multi-user storage service, make the token opaque or use a per-user/store ordering value.
Low: negative page_size is still unvalidated, and this commit makes page_size = -1 silently return an empty successful page because fetch_limit becomes 0 (rust/impls/src/postgres_store.rs:688). Other negative values still become PostgreSQL limit errors. Reject page_size < 0 with
InvalidRequestError before computing limit.

Curious what you think of the first one here, it's a good point I find.

benthecarman · 2026-05-04T18:58:57Z

Curious what you think of the first one here, it's a good point I find.

I had similar thought but I don't really seem the harm. Maybe you can infer something about user activity but not really the end of the world. Doing a per user token doesn't totally work because the big serial is across the whole table, we could encrypt it or something but it is nice that the user can easily compare between 2 tokens

tankyleo

Sounds good thanks again for the PR

benthecarman requested a review from tankyleo April 2, 2026 01:55

benthecarman force-pushed the creation-pagination branch from defad39 to cb70f98 Compare April 2, 2026 01:58

benthecarman self-assigned this Apr 2, 2026

benthecarman added this to Weekly Goals Apr 2, 2026

benthecarman mentioned this pull request Apr 3, 2026

Add 60 min timeout to ldk-node-integration CI #97

Merged

tankyleo reviewed Apr 4, 2026

View reviewed changes

Comment thread rust/impls/src/migrations.rs Outdated

Comment thread rust/impls/src/migrations.rs Outdated

Comment thread rust/impls/src/postgres_store.rs Outdated

Comment thread rust/impls/src/postgres_store.rs Outdated

tankyleo reviewed Apr 4, 2026

View reviewed changes

Comment thread rust/api/src/kv_store_tests.rs Outdated

Comment thread rust/api/src/kv_store_tests.rs

tankyleo reviewed Apr 4, 2026

View reviewed changes

Comment thread rust/impls/src/postgres_store.rs Outdated

benthecarman force-pushed the creation-pagination branch 2 times, most recently from dc205c9 to cd32024 Compare April 4, 2026 18:18

tankyleo reviewed Apr 6, 2026

View reviewed changes

benthecarman requested a review from tankyleo April 7, 2026 03:35

benthecarman force-pushed the creation-pagination branch from 6622747 to c02dcda Compare April 8, 2026 02:16

benthecarman mentioned this pull request Apr 8, 2026

Add PaginatedKVStore support to VssStore lightningdevkit/ldk-node#864

Open

tnull reviewed Apr 8, 2026

View reviewed changes

benthecarman force-pushed the creation-pagination branch from c02dcda to 34d4241 Compare April 8, 2026 19:46

tankyleo reviewed Apr 9, 2026

View reviewed changes

benthecarman force-pushed the creation-pagination branch from 34d4241 to e17c676 Compare April 9, 2026 22:00

benthecarman requested a review from tankyleo April 9, 2026 22:01

tankyleo reviewed Apr 9, 2026

View reviewed changes

Comment thread rust/api/src/types.rs Outdated

benthecarman force-pushed the creation-pagination branch from 0bddc2a to c70f65e Compare April 14, 2026 21:24

tankyleo requested a review from tnull April 14, 2026 21:40

tnull reviewed Apr 15, 2026

View reviewed changes

Comment thread rust/impls/src/postgres_store.rs Outdated

benthecarman force-pushed the creation-pagination branch 2 times, most recently from 0e4cd21 to b67174e Compare April 15, 2026 17:11

tankyleo reviewed Apr 15, 2026

View reviewed changes

tankyleo requested a review from tnull April 15, 2026 17:46

benthecarman force-pushed the creation-pagination branch from b67174e to e618d5f Compare April 15, 2026 20:52

tnull reviewed Apr 16, 2026

View reviewed changes

benthecarman force-pushed the creation-pagination branch from e618d5f to 9d87571 Compare April 16, 2026 20:29

benthecarman force-pushed the creation-pagination branch from 9d87571 to 4c0b480 Compare April 16, 2026 20:45

tankyleo reviewed Apr 16, 2026

View reviewed changes

tankyleo requested a review from tnull April 16, 2026 21:22

tnull reviewed Apr 20, 2026

View reviewed changes

tnull mentioned this pull request Apr 20, 2026

Add Postgres database lightningdevkit/ldk-node#863

Open

tankyleo mentioned this pull request Apr 28, 2026

Add health check API endpoint #98

Open

benthecarman requested a review from tnull May 4, 2026 01:20

benthecarman force-pushed the creation-pagination branch from 4c0b480 to e7566b6 Compare May 4, 2026 14:29

tankyleo self-requested a review May 4, 2026 15:24

tankyleo reviewed May 4, 2026

View reviewed changes

tankyleo approved these changes May 4, 2026

View reviewed changes

Conversation

benthecarman commented Apr 2, 2026

Uh oh!

ldk-reviews-bot commented Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ldk-reviews-bot commented Apr 4, 2026

Uh oh!

tankyleo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tankyleo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

benthecarman commented Apr 4, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tankyleo commented Apr 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ldk-reviews-bot commented Apr 9, 2026

Uh oh!

tankyleo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tankyleo commented Apr 9, 2026

Uh oh!

G8XSU commented Apr 9, 2026

Uh oh!

benthecarman commented Apr 9, 2026

Uh oh!

tankyleo commented Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benthecarman commented Apr 9, 2026

Uh oh!

Uh oh!

Uh oh!

tankyleo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ldk-reviews-bot commented Apr 2, 2026 •

edited

Loading

tankyleo commented Apr 6, 2026 •

edited

Loading

tankyleo commented Apr 9, 2026 •

edited

Loading

tankyleo left a comment •

edited

Loading